Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce QuarkusProject and Quarkus project zipping in codegen #9631

Merged
merged 5 commits into from
May 28, 2020

Conversation

ia3andy
Copy link
Contributor

@ia3andy ia3andy commented May 27, 2020

This is part of #8178

This is a good one :-), things starts to get nicer and cleaner (but not quite there yet ^^)..

Introduce QuarkusProject in codegen to reduce ProjectWriter & BuildTool coupling, it will slowly replace BuildFile and ProjectWriter.

I also uniformised the quarkus-cli (I am not sure if it's used) by giving the project folder instead of the pom.xml, it should now also work with Gradle.

Deleted ZipProjectWriter which is not used and will be replaced by a way to zip a Quarkus project folder.

@boring-cyborg boring-cyborg bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle area/maven area/platform Issues related to definition and interaction with Quarkus Platform labels May 27, 2020
@ia3andy ia3andy requested a review from aloubyansky May 27, 2020 11:26
@ia3andy ia3andy force-pushed the introduce-quarkus-project branch from 3452ea3 to 1dc58e9 Compare May 27, 2020 11:41
@ia3andy ia3andy changed the title Introduce QuarkusProject in codegen to reduce ProjectWriter & BuildTool coupling [wip] Introduce QuarkusProject in codegen to reduce ProjectWriter & BuildTool coupling May 27, 2020
@ia3andy
Copy link
Contributor Author

ia3andy commented May 27, 2020

Putting in WIP, I am going to introduce the new folder zipping as another commit.

@ia3andy ia3andy changed the title [wip] Introduce QuarkusProject in codegen to reduce ProjectWriter & BuildTool coupling [wip] Introduce QuarkusProject and Quarkus project zipping in codegen May 27, 2020
@ia3andy
Copy link
Contributor Author

ia3andy commented May 27, 2020

@aloubyansky there is a bug with the maven plugin due to ProjectWriter.close(), I'll figure it out tomorrow..

@ia3andy
Copy link
Contributor Author

ia3andy commented May 27, 2020

ok, found it, it's nothing, I'll push tomorrow..

throw new IllegalStateException("Project writer has not been provided");
}
final ProjectWriter projectWriter = new FileProjectWriter(
invocation.getQuarkusProject().getProjectFolderPath().toFile());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note, previously a single invocation could have been passed to multiple handlers (e.g. create followed by add extensions) that would use the same project writer instance. It doesn't mean it has to stay that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not there yet, but to make it more consistent and simple, my intent is to go toward atomic operations (without having to deal with the writer close/save from outside):

  • create (directly with or without extensions)
  • add extension(s)
  • remove extension(s)

Using the same writer would make it more performant when doing multiple of those, but I guess there is no use case for it.

One good example of the risk of having this close/save handled externally is that (as I did without knowing) having multiple writer and closing the one without the modification after ending up with reseting the modification 😅

WDYT?

@aloubyansky
Copy link
Member

One thing to keep in mind is the public API should be backward compatible. Our plugins are looking for the latest platform in a given version range. So, inside that range the API should stay compatible.
The issue you ran into with 1.4.2.Final is an example of what shouldn't happen to a user.
I am not going to insist on keeping the current API now. But you should keep this point in mind for the future.
BTW, the current version range for the platform is up to 2.0. We did consider changing it to a micro. E.g. [1.5.0, 1.6.a)

@aloubyansky
Copy link
Member

The PR is marked as a WIP, so I am not approving it yet.

@ia3andy
Copy link
Contributor Author

ia3andy commented May 28, 2020

One thing to keep in mind is the public API should be backward compatible. Our plugins are looking for the latest platform in a given version range. So, inside that range the API should stay compatible.
The issue you ran into with 1.4.2.Final is an example of what shouldn't happen to a user.
I am not going to insist on keeping the current API now. But you should keep this point in mind for the future.
BTW, the current version range for the platform is up to 2.0. We did consider changing it to a micro. E.g. [1.5.0, 1.6.a)

Yeah, I was wondering... It would be good to be allowed to break it in one shot for this refactor? I am not sure if this part of the code is used externally by other than code.quarkus.io?

Once it's is clean, we can of course lock it..

@ia3andy ia3andy changed the title [wip] Introduce QuarkusProject and Quarkus project zipping in codegen Introduce QuarkusProject and Quarkus project zipping in codegen May 28, 2020
@ia3andy
Copy link
Contributor Author

ia3andy commented May 28, 2020

@aloubyansky Ok I added the project zip compression utils f44f980 (inspired from code.quarkus.io with exec support for wrapper files)

And I fixed the BuildFile closing bug by removing the close in BuildFileMojoBase, the close has to be done by the underlying command handler.

ia3andy added 3 commits May 28, 2020 11:42
…Tool coupling

This is part of quarkusio#8178

This is a good one :-), things starts to get nicer and cleaner (but not quite there yet ^^)..
@ia3andy ia3andy force-pushed the introduce-quarkus-project branch from f44f980 to d3b2456 Compare May 28, 2020 09:42
@ia3andy ia3andy force-pushed the introduce-quarkus-project branch from d3b2456 to 2e7b78b Compare May 28, 2020 13:41
@ia3andy
Copy link
Contributor Author

ia3andy commented May 28, 2020

@aloubyansky CI is failing because quay seems to be down..

@aloubyansky aloubyansky merged commit d5c94c1 into quarkusio:master May 28, 2020
@ia3andy ia3andy deleted the introduce-quarkus-project branch May 29, 2020 04:54
@maxandersen
Copy link
Member

Yeah, I was wondering... It would be good to be allowed to break it in one shot for this refactor? I am not sure if this part of the code is used externally by other than code.quarkus.io?

this is something we should discuss the timing off. I fully agree it needs to happen - but want to try and make it happen in just one release if at all possible.

@maxandersen
Copy link
Member

BTW, the current version range for the platform is up to 2.0. We did consider changing it to a micro. E.g. [1.5.0, 1.6.a)

shouldn't we just do this ? it would be [1.5.0,1.6.0)

other only alternative I can think of is the introduction of a separate file where we externalize the range we check so we can adjust it after release and be able to update it once we know incompatibility happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle area/maven area/platform Issues related to definition and interaction with Quarkus Platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants